Skip to content

refactor(dynacell/eval): simplify the #511 prediction-provenance code - #514

Merged
alxndrkalinin merged 16 commits into
dynacell-modelsfrom
refactor/pred-cache-provenance-simplify
Sep 30, 2026
Merged

alxndrkalinin merged 16 commits into
dynacell-modelsfrom
refactor/pred-cache-provenance-simplify

Conversation

@alxndrkalinin

@alxndrkalinin alxndrkalinin commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This is a cleanup pass (/simplify) over the prediction-provenance code that #511 added to the dynacell eval pred cache. #511's behaviour is unchanged on every production store, manifest and metrics sidecar measured (below). There are also two small fixes and one test, all found in this branch's internal review.

Simplifications and speed-ups (commits 1–11):

  • Cache manifests are read and written with libyaml's C loader and dumper. On a real manifest with 6 families × 100 positions and 600 sources (73 kB), save+load drops from 0.299 s to 0.022 s (median of 5).
  • One shared JSON sha256_12 helper.
  • The flat zarr-v2 .-separated chunk-key fallback scan is dropped. No evaluated store uses that layout: all 8 zarr-v2 prediction stores use /.
  • Single positions are read through prediction_sources.
  • Legacy caches and metrics sidecars are dated by one rule.
  • The prediction stamp is checked inside the sidecar reader (metrics_provenance_matches(..., prediction_sources=)).
  • Manifest entries are walked through one helper (_artifact_entries / _artifact_identity).
  • _merge_manifests requires the written set.
  • Prediction sources are read once per condition: the caller's snapshot is passed as prediction_snapshot and copied by the pred context.
  • Docstring corrections and shared test predicates.

Review fixes (commits 12–15):

  • 22df884b: write manifests exactly as OmegaConf.save did. The plain CSafeDumper differs from OmegaConf.save in three ways:

    • it leaves a str such as the hex sha12 marker 4e1234567890 unquoted, and OmegaConf's loader (or any tool reading a manifest with it) parses that as a float;
    • it emits &id001 aliases when two entries share one object, as ctx.spacing does;
    • it escapes non-ASCII text.

    A dumper subclass now ignores aliases and quotes the way OmegaConf's representer does. A test pins the bytes to OmegaConf.save. Known positive: plain CSafeDumper output ≠ OmegaConf's, and OmegaConf reads its marker back as a float.

  • 866996be: replace manifests atomically (temp file + os.replace). init_cache_context reads the manifest without the lock, so a job starting during another job's flush could read a truncated file. OmegaConf silently turned that into an empty cache; the libyaml loader raises. A new test kills a writer mid-dump. The old in-place writer fails that test and leaves cache_schema_version: 1\nartif behind; the new writer leaves the old file intact.

    • Lock files live under .locks/, so the rename leaves locking alone.
    • Cache dirs are setgid with default ACLs, so the replacement keeps its group and permissions.
  • 37e6b412 (Copilot): the manifest temp file is created exclusively ("x") under a uuid4 name, so a path or symlink planted in the group-writable cache dir is refused, not truncated or followed.

  • 444b34d9: the snapshot test now adds a position on demand and checks that the caller's snapshot is unchanged. It fails without the dict() copy.

Equivalence on production data (MEASURED, base dfcf9a53 vs this branch, separate processes, eval venv)

Check Scope Identical Different
prediction_sources 48 stores (all 8 zarr-v2 + 40 stratified v3), 2262 positions 48 stores / 2262 positions, all digests equal 0
Manifest load (objects and types) 1224 manifests (every pred cache in the grouped configs + 30 GT) 1223 1: a live spotlight eval rewrote it between the two reads
Manifest save, same object, OLD vs NEW bytes 1224 1224 0
Mixed writers (OLD reads NEW's file and vice versa) 1224 × 2 all 0
Legacy one-time upgrade (in memory, on copies) 51 manifests, 14,700 source entries 51 0
Final-metrics reuse gate 60 cases: 4 digest sidecars, 8 legacy sidecars, perturbations 60 agree 0

Every comparison flagged its own known-positive perturbation (for example, one written_ns + 1 was caught as 1 differing position). The manifest rows were measured before 22df884b; that commit changes output only for strings no production manifest holds yet (the round-trip test covers those).

Review

There were two independent review passes: a commit-by-commit code review and the production-equivalence run above.

Fixed: the three commits above.

Accepted and noted:

  • A flat .-separated zarr-v2 store with a blank first chunk now reads as undatable (written_ns=None). It is rebuilt once, and after that a code-only re-predict of it goes undetected. No such store exists (all 8 v2 stores use /), and the writer only writes v3.
  • A manifest entry with sources: null now raises AttributeError instead of being treated as a cache miss. That fits the raise-don't-fall-back rule, and the writer never produces it.
  • json_sha256_12 uses default=str for all callers, because the encoder config holds a Path. Markers come from JSON attributes, so they can't reach the fallback.
  • The final-metrics gate now reads the prediction store before the cubic/CP stamp. A missing store therefore raises earlier; fix(dynacell/eval): invalidate pred caches and metrics when the prediction changes #511 already made a missing store raise at the gate.

Tests

Local runs at 37e6b412 (tools/ at 444b34d9; the last commit touches only save_manifest and its test), eval venv cpdino-eval-cubic090a2, worktree sources on PYTHONPATH, separate processes, junitxml counts:

suite collected passed skipped failed / errors
applications/dynacell/tests 3683 3512 171 0 / 0
applications/dynacell/src/dynacell/evaluation 173 173 0 0 / 0
applications/dynacell/tools 226 226 0 0 / 0

New or changed tests: test_save_manifest_writes_what_omegaconf_wrote, test_save_manifest_interrupted_leaves_the_old_file, test_save_manifest_never_writes_through_a_planted_temp_path (Copilot finding, 37e6b412), test_pred_context_uses_the_callers_snapshot (added in 63933855, strengthened in 444b34d9) and one unit test in provenance_test.py. Each review-fix test fails against the code it guards (old dumper, in-place writer, "w" temp open, no dict() copy).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF

alxndrkalinin and others added 14 commits September 30, 2026 12:30
The manifest is saved and reloaded after every FOV, and #511's
per-position sources made it several times larger. OmegaConf round-trips
it through its own node tree plus the pure-Python YAML emitter.

PyYAML's C loader and dumper produce byte-identical files and equal
dicts. On a real 6-family x 100-position manifest with 600 sources,
save+load drops from 0.299 s to 0.022 s. Interpolation resolution is
dropped with OmegaConf; no production manifest contains `${`. The
pure-Python classes remain the fallback for a PyYAML without libyaml.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The marker digest, the prediction-sources digest and the encoder-config
hash each built the same key-sorted JSON payload and truncated sha256
by hand. json_sha256_12 does it once; digests are unchanged (checked on
a real marker, a real 100-position source snapshot and a non-JSON
encoder value, which default=str still serializes).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The bounded scandir for `.`-separated keys served only flat zarr v2
layouts, and every evaluated non-v3 prediction store uses `/`
separators, so nothing reaches it. A flat store now resolves only its
exact chunk key; when that chunk is absent the position reads as
unwritten, which rebuilds a legacy cache entry over it once. The `/`
prefix fallback and the exact-key lookup for every layout stay.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
…rces

prediction_source duplicated prediction_sources for one position; a
positions= argument covers it. The on-demand read for a position that
appeared after the snapshot now takes the store and channel from the
manifest's pred identity, which init_cache_context already seeds and
validates, so _CacheContext no longer carries its own copy of them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The legacy manifest upgrade and the final-metrics gate each spelled out
"a position is verified iff its first stored chunk exists and is no
newer than the legacy record's time". source_predates states it once;
the upgrade checks the missing-built_at case once instead of per
position, and drops an isinstance guard on the dict it just wrote.

The upgrade also no longer builds per-family identities it discards:
_artifact_checks splits into _artifact_entries (kind, label, manifest
keys) and _artifact_identity, which only the param check needs.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
…ader

metrics_provenance_matches already reads metrics_provenance.json; the
final-metrics gate then read it a second time, without the is_file()
guard, to compare the prediction digest. It now takes the current
prediction_sources and does both checks from one read, with the same
semantics: the stamped digest must match, and an unstamped sidecar is
judged by date through source_predates against its mtime.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The five cache writers each repeated _update_manifest_entry, then
_record_position, then mark_manifest_dirty; _record_write does the three.
_record_position absorbs _add_position's separate manifest walk and uses
_manifest_leaf, and _source_current takes the entry's keys instead of
every caller walking to the entry first, which drops its isinstance
guards on values that are always dicts. _instance_keys names the
instance-label entry once for the writer, instance_cache_hit and the
family list.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
flush_manifest, the only caller, always passes the positions this
process wrote, and the source merge is only correct with them: a
defaulted empty set would let the on-disk copy win for positions this
process just rebuilt. Making the argument required keeps a future
caller from dropping it silently.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
evaluate_model and the grouped driver read every position's sources to
stamp the metrics, then init_cache_context read them all again for the
pred cache. The caller's snapshot now flows through evaluate_predictions
and init_cache_contexts into the pred context, which copies it (it adds
positions read on demand); the stamp hashes the same snapshot. The
final-metrics gate and process-executor workers still read their own.

The save_metrics / write_metrics_provenance kwarg becomes
prediction_digest: prediction_sources_sha256_12 shadowed the function
of that name. The sidecar key is unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The README still dated a position by its t=0 chunk; it is the earliest
stored timepoint since blank outputs store none at t=0. The feature
prefetch comment named force_recompute, but the guard is now any
position that is rewritten anyway. Comments that restated the
_source_current call go, and the legacy-dating docstrings keep their
reasoning without the campaign's dates and counts.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
Four places in the prediction-identity tests re-derived "zarr metadata
vs chunk file" by hand; _is_metadata and _chunks now define it once. The
pipeline cache-reuse test builds its prediction store with
make_hcs_plate like the other eval tests, dropping its own iohub setup.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
The plain libyaml SafeDumper differs from OmegaConf.save in three
ways that matter here: it leaves a str such as the hex sha12 marker
4e1234567890 unquoted, which OmegaConf's loader (and any tool that
reads a manifest with it) parses as a float; it emits &id001 aliases
when two entries share one object, as ctx.spacing does; and it
escapes non-ASCII text. A dumper subclass now ignores aliases and
single-quotes any str that parses as a bool, int or float, as
OmegaConf's own representer does, and allow_unicode is set. A new
test pins the output byte-for-byte to OmegaConf.save on those cases.

Found in the internal review of this branch; all 1224 production
manifests were already byte-identical, since none holds such a str yet.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
save_manifest truncated the manifest in place while
init_cache_context reads it without the manifest lock, so an eval
starting during another job's flush, or after a writer died mid-dump,
read a torn file. OmegaConf turned an empty read into an empty cache;
the libyaml loader on this branch raises StaleCacheError instead.
Writing to a sibling temp file and renaming it over the manifest
leaves readers the old file or the new one. The lock files live under
.locks/, so the rename does not disturb locking, and cache dirs are
setgid with default ACLs, so the replacement keeps the group and
permissions.

Found in the internal review of this branch.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
…ler's snapshot

The metrics digest is computed from the snapshot the caller read before
scoring, and the pred context adds positions that appear later to its
own copy. The old test only checked identity (is not); it now adds a
position on demand and checks the caller's dict is unchanged. Dropping
the dict() copy in init_cache_context makes it fail.

Found in the internal review of this branch.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The predictable temporary manifest path is unsafe in group-writable cache directories.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Simplifies DynaCell evaluation provenance while preserving prediction-aware cache invalidation.

Changes:

  • Consolidates prediction snapshots, provenance hashing, and legacy dating.
  • Replaces OmegaConf manifest I/O with faster atomic PyYAML operations.
  • Refactors artifact bookkeeping and expands regression coverage.
File Description
applications/​dynacell/​tools/​scan_nan_microssim_caches_test.py Updates provenance API usage.
applications/​dynacell/​tests/​test_pipeline_cache.py Updates digest argument names.
applications/​dynacell/​tests/​test_evaluation_timepoint_guard.py Adapts snapshot-aware test stubs.
applications/​dynacell/​tests/​test_evaluation_pred_cache_identity.py Tests snapshot isolation and supported layouts.
applications/​dynacell/​tests/​test_evaluation_pipeline.py Uses shared plate fixtures and digest API.
applications/​dynacell/​tests/​test_evaluation_cp_scaler_selection.py Updates grouped-evaluation stubs.
applications/​dynacell/​tests/​test_evaluation_cache.py Tests manifest compatibility and atomic replacement.
applications/​dynacell/​tests/​test_evaluate_compose.py Updates evaluation hook signature.
applications/​dynacell/​src/​dynacell/​evaluation/​README.md Corrects prediction-source behavior documentation.
applications/​dynacell/​src/​dynacell/​evaluation/​provenance.py Integrates prediction validation into sidecar checks.
applications/​dynacell/​src/​dynacell/​evaluation/​provenance_test.py Covers digest and legacy-date matching.
applications/​dynacell/​src/​dynacell/​evaluation/​pipeline.py Shares prediction snapshots through evaluation.
applications/​dynacell/​src/​dynacell/​evaluation/​pipeline_cache.py Consolidates artifact and source bookkeeping.
applications/​dynacell/​src/​dynacell/​evaluation/​model_loader.py Passes snapshots into prediction cache contexts.
applications/​dynacell/​src/​dynacell/​evaluation/​instance_metrics_test.py Updates provenance argument naming.
applications/​dynacell/​src/​dynacell/​evaluation/​cache.py Adds YAML I/O, atomic writes, and shared hashing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

alxndrkalinin and others added 2 commits September 30, 2026 13:45
… random name

The temp name was predictable (host + pid) and opened with "w", which
truncates and follows whatever already sits at that path. Cache dirs
are group-writable, so another process could plant a symlink there
and have the writer truncate a file it points to; a stale temp could
also collide after pid reuse. The name is now a uuid4 and the file is
opened with "x" (O_CREAT | O_EXCL), which refuses any existing path,
symlinks included. On that refusal the path is left alone rather than
unlinked. Permissions still come from umask plus the directory's
default ACL, unlike mkstemp's fixed 0600.

A test plants a symlink at the temp name and checks that save_manifest
raises, the target is untouched and the symlink remains; with "w" it
does not raise.

Addresses Copilot review on #514.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
…fy branch

#515 widened the cubic check in metrics_provenance_matches to an
equivalence class; this branch had already moved the prediction stamp
into the same function. Kept both: the class check, then the digest
check. The only textual conflict was the Returns docstring.

#515's new test_equivalent_environment_reuses_the_declared_pins_cache
called the gate without prediction_sources, which this branch makes
required; it now passes _SOURCES, matching the _DIGEST that
_stamp_cubic writes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XvkM1gKifWmoDPq8Drd4JF
@alxndrkalinin
alxndrkalinin merged commit 7ab3547 into dynacell-models Sep 30, 2026
26 checks passed
@alxndrkalinin
alxndrkalinin deleted the refactor/pred-cache-provenance-simplify branch September 30, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants